Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

proc/core: implementing coredump functionality for ARM64 #1774

Merged
merged 2 commits into from
Feb 17, 2020
Merged

proc/core: implementing coredump functionality for ARM64 #1774

merged 2 commits into from
Feb 17, 2020

Conversation

ossdev07
Copy link
Contributor

Implementing coredump functionality for ARM64 , now it can be checked by using "dlv core " command.
Floating point registers are not handled yet, working on it.

@ossdev07 ossdev07 changed the title go-delve: implementing coredump functionality for ARM64 proc/core: implementing coredump functionality for ARM64 Nov 28, 2019
@hengwu0 hengwu0 mentioned this pull request Nov 28, 2019
Closed
aarzilli
aarzilli reviewed Nov 29, 2019
View reviewed changes
@ossdev07 ossdev07 force-pushed the coredump_arm64 branch 3 times, most recently from be0bc33 to 5be350d Compare December 3, 2019 10:36
aarzilli
aarzilli reviewed Dec 4, 2019
View reviewed changes
Copy link
Member

@aarzilli aarzilli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that all tests for core are stacktrace dependent and disabled (because arm64 doesn't have functioning stacktraces) so this will have to wait for stacktraces on arm64 before being merged.

@hengwu0
Copy link
Contributor

hengwu0 commented Dec 5, 2019

this will have to wait for stacktraces on arm64 before being merged.

arm64 stacktraces is almost done and there are some bugfixes work left.

aarzilli
aarzilli reviewed Dec 5, 2019
View reviewed changes
aarzilli
aarzilli reviewed Dec 6, 2019
View reviewed changes
Copy link
Member

@aarzilli aarzilli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, as I said I'd wait for stacktraces before merging so we can test it. We rarely touch core so there's little risk of code rot.

@ossdev07
Copy link
Contributor Author

@aarzilli ,kindly confirm, is stacktraces implemented?
So that, this PR can be merged.

@aarzilli
Copy link
Member

kindly confirm, is stacktraces implemented?

No.

@hengwu0
Copy link
Contributor

hengwu0 commented Jan 9, 2020

I mean linutil.Decode() should be removed since there is new ARM64PtraceFpRegs.Decode(). They are duplicated codes.

@hengwu0
Copy link
Contributor

hengwu0 commented Jan 14, 2020

LGTM

derekparker
derekparker reviewed Feb 11, 2020
View reviewed changes
Copy link
Member

@derekparker derekparker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small nits, overall looks good.

@ossdev07
Copy link
Contributor Author

ossdev07 commented Feb 13, 2020 •
edited
Loading

@derekparker it's long pending PR so can we do all refactoring in a new PR and let this PR go in master branch
Please share your suggestion ..

@derekparker
Copy link
Member

@ossdev07 there are merge conflicts anyways. I'd say resolve the conflicts and the feedback at the same time and push your changes. After that I will merge quickly. Sorry for the delay.

@derekparker derekparker mentioned this pull request Feb 13, 2020
Closed
aarzilli and others added 2 commits February 17, 2020 14:02
@aarzilli @odidev
proc/native: optimize native.status through buffering (go-delve#1865)
fea80f1 
Benchmark before:

BenchmarkConditionalBreakpoints-4              1        15649407130 ns/op

Benchmark after:

BenchmarkConditionalBreakpoints-4   	       1	14586710018 ns/op

Conditional breakpoint evaluation 1.56ms -> 1.45ms

Updates go-delve#1549
@ossdev07
proc/core: Review Comments Incorporated
5193adf 
Signed-off-by: ossdev07 <[email protected]>
@ossdev07
Copy link
Contributor Author

@ossdev07 there are merge conflicts anyways. I'd say resolve the conflicts and the feedback at the same time and push your changes. After that I will merge quickly. Sorry for the delay.

@derekparker Review comments are incorporated. please have a look

derekparker
derekparker approved these changes Feb 17, 2020
View reviewed changes
Copy link
Member

@derekparker derekparker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@derekparker derekparker merged commit a83d1c1 into go-delve:master Feb 17, 2020
cgxxv pushed a commit to cgxxv/delve that referenced this pull request Mar 25, 2022
@ossdev07 @aarzilli
proc/core: implementing coredump functionality for ARM64 (go-delve#1774)
910d0ee 
* proc/native: optimize native.status through buffering (go-delve#1865)

Benchmark before:

BenchmarkConditionalBreakpoints-4              1        15649407130 ns/op

Benchmark after:

BenchmarkConditionalBreakpoints-4   	       1	14586710018 ns/op

Conditional breakpoint evaluation 1.56ms -> 1.45ms

Updates go-delve#1549

* proc/core: Review Comments Incorporated

Signed-off-by: ossdev07 <[email protected]>

Co-authored-by: Alessandro Arzilli <[email protected]>
abner-chenc pushed a commit to loongson/delve that referenced this pull request Mar 1, 2024
@ossdev07 @aarzilli @abner-chenc
proc/core: implementing coredump functionality for ARM64 (go-delve#1774)
1ee2523 
* proc/native: optimize native.status through buffering (go-delve#1865)

Benchmark before:

BenchmarkConditionalBreakpoints-4              1        15649407130 ns/op

Benchmark after:

BenchmarkConditionalBreakpoints-4   	       1	14586710018 ns/op

Conditional breakpoint evaluation 1.56ms -> 1.45ms

Updates go-delve#1549

* proc/core: Review Comments Incorporated

Signed-off-by: ossdev07 <[email protected]>

Co-authored-by: Alessandro Arzilli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aarzilli aarzilli aarzilli left review comments

@hengwu0 hengwu0 hengwu0 left review comments

@derekparker derekparker derekparker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ossdev07 @hengwu0 @aarzilli @derekparker